Conversation
|
Still needs some work.. Video_2025-10-21_18-02-04.mp4 |
e0fb1e9 to
4ee7344
Compare
… and manual_force_sale_max_qty (imp user UI)
… other module & pep8
Currently translated at 100.0% (54 of 54 strings) Translation: sale-workflow-14.0/sale-workflow-14.0-sale_restricted_qty Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-14-0/sale-workflow-14-0-sale_restricted_qty/it/
Currently translated at 100.0% (54 of 54 strings) Translation: sale-workflow-14.0/sale-workflow-14.0-sale_restricted_qty Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-14-0/sale-workflow-14-0-sale_restricted_qty/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-workflow-16.0/sale-workflow-16.0-sale_restricted_qty Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_restricted_qty/
Currently translated at 100.0% (51 of 51 strings) Translation: sale-workflow-16.0/sale-workflow-16.0-sale_restricted_qty Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_restricted_qty/it/
0a64620 to
e8dc0b8
Compare
This commit brings the enhanced functionality from the improved version while maintaining Odoo 18.0 compatibility. Includes: - Enhanced model architecture with better inheritance - Improved field names and functionality - Better UI with proper decorations and warnings - Odoo 18.0 view compatibility fixes [ADD] sale_restricted_qty: Add German and Dutch translation files Add skeleton translation files for German (de.po) and Dutch (nl.po) languages. These files follow OCA standards for internationalization: - Proper headers with language codes - Correct plural forms for respective languages - Consistent with existing translation files in the module
Merge duplicated 'sale.order.line' model inheritance from sale.py and sale_order_line.py The improved model structure from sale_order_line.py replaces the original migration model. This resolves pylint warning R8180 about considering merging classes inherited to the same model.
…nforced Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Add feature to automatically populate sale order line quantity with minimum quantity when: - Minimum quantity is set on the product - Minimum quantity restriction is enforced (restrict_min_qty = True) - Current quantity is 0 or not set This improves user experience by providing sensible defaults while maintaining validation for minimum quantities. Includes comprehensive unit tests covering: - Auto-population when enforced - No auto-population when not enforced - Preserving existing quantities - Onchange behavior simulation
9eebebe to
15f837b
Compare
|
/ocabot migration sale_restricted_qty |
|
The migration issue (#3344) has not been updated to reference the current pull request because a previous pull request (#3609) is not closed. |
| if not self.parent_id: | ||
| return super()._get_is_sale_inherited_min_qty_set() | ||
|
|
||
| self.ensure_one() |
There was a problem hiding this comment.
Why here and not at the function beginning ?
| class ProductMinMultipleMixin(models.AbstractModel): | ||
|
|
||
| class ProductRestrictedQtyMixin(models.AbstractModel): | ||
| _name = "product.restricted.qty.mixin" |
There was a problem hiding this comment.
As this is 'sale' oriented, I would have renamed "sale.product..."
| self.ensure_one() | ||
| return self.categ_id.sale_min_qty | ||
|
|
||
| @api.depends("categ_id.is_sale_restrict_min_qty_set") |
There was a problem hiding this comment.
To avoid all these overrides (for only depends()) in products and in categories, I would have added a private field in mixin like _sale_restricted_qty_field that would have been called in mixin depends.
…e overrides Address PR OCA#3959 review comments: - Rename mixin to sale.product.restricted.qty.mixin - Move ensure_one() to beginning of _get_* methods - Add _sale_restricted_qty_parent_field class attribute with callable @api.depends to avoid 24+ method overrides per child model
|
@rousseldenis Thanks for the review, the items hass been adressed. Is it ok now? Can we push this one forward? |
rousseldenis
left a comment
There was a problem hiding this comment.
Seems lighter. LGTM
|
@rousseldenis Can you please merge? |
alexey-pelykh
left a comment
There was a problem hiding this comment.
Solid migration and redesign. The own/inherited/computed pattern for hierarchical quantity constraints is well-structured and the test coverage is thorough.
A few observations:
-
Typo in .pot:
"Sale Miltiple qty"should be"Sale Multiple qty"-- this propagates to all .po files. Likely an auto-extraction artifact, but worth regenerating. -
is_sale_own_*_setfields not stored on mixin:is_sale_own_min_qty_set,is_sale_own_max_qty_set,is_sale_own_multiple_of_qty_setare plainfields.Boolean()withoutstore=Trueon the mixin definition. This is fine since they default to stored, just noting for consistency with the computed booleans that have explicitstore=True. -
_compute_restricted_qty_constraintsnot stored: Theis_below_min_qty,is_above_max_qty,is_not_multiple_of_qtyfields onsale.order.lineare computed withoutstore=True. This is intentional (avoids stale data on qty changes), but means every form/list render triggers recomputation. Acceptable for sale order line volumes. -
Float modulo in multiple-of check:
qty % line.multiple_of_qty != 0uses float modulo which can have precision issues. Consider usingfloat_compareorfloat_roundfromodoo.toolsfor robustness, though for typical order quantities this is unlikely to cause real issues. -
Migration handles
multiple_ofrestrict: The migration script handlesmin_qtyandmax_qtyrestrict fields but notmultiple_of_qtyrestrict (thefor field in ["min_qty", "max_qty"]loop inmigrate_selection_and_flags). If the previous version had amanual_force_sale_multiple_qtycolumn, it would be missed. If it didn't exist, this is fine. -
Legacy .pot file:
sale_order_min_qty.potis included alongsidesale_restricted_qty.pot. Should this be removed if the module was renamed?
Overall LGTM. The architecture is clean, tests cover the inheritance chain well including UoM conversion edge cases, and the migration script handles the data transition correctly.
| "restrict_max_qty", | ||
| "is_multiple_of_qty_set", | ||
| "multiple_of_qty", | ||
| "restrict_multiple_of_qty", |
There was a problem hiding this comment.
Float modulo (qty % line.multiple_of_qty) can produce precision artifacts with floats. Consider float_is_zero(qty % line.multiple_of_qty, precision_rounding=...) from odoo.tools.float_utils for robustness. Minor concern for typical order quantities.
| """ | ||
| for field in ["min_qty", "max_qty"]: | ||
| old_col = f"manual_force_sale_{field}" | ||
| new_col = f"sale_own_restrict_{field}" |
There was a problem hiding this comment.
This loop handles min_qty and max_qty restrict fields. Was there a manual_force_sale_multiple_qty column in the previous version? If so, it would need handling here too.
|
@rousseldenis I think the failing tests are not related to this PR. Moreover with 4 approved reviews, it may be time to merge it! Thanks |
Merge will not pass if tests are red. Usually, someone (a good will) will fix them in order to merge 😄 |
|
@bosd can you rebase your branch? |
BhaveshHeliconia
left a comment
There was a problem hiding this comment.
Functional review LGTM!
|
Hi all, I'm a bit surprised the way this PR was done. However, updating the sources in our project, I see that the UX completely changed. I find it a bit strange to include an old open PR in the migration process of a higher version, without explicitly mentioning it in the description. In my opinion before merging this 18.0 PR we should finish the 17.0 UX improvements PR from @alexey-pelykh so that we're sure it doesn't change, merge it, and then merge this 18.0 PR. What do you think? |
| RESTRICTION_ENABLED = "1" | ||
| RESTRICTION_DISABLED = "0" | ||
| RESTRICTION_SELECTION = [ | ||
| (RESTRICTION_ENABLED, "Yes"), | ||
| (RESTRICTION_DISABLED, "No"), | ||
| (RESTRICTION_ENABLED, "Blocking"), | ||
| (RESTRICTION_DISABLED, "Warning"), |
There was a problem hiding this comment.
Just repeating the comment I've added on the 17.0 PR:
Would be better to have real chars as selection values, it eases the code comprehension:
I would suggest something like RESTRICTION_ENABLED = "enabled", "restricted" or "blocking", and RESTRICTION_DISABLED = "disabled", "unrestricted" or "warning".
There was a problem hiding this comment.
@marielejeune Good point!
I would propose to do that in a follow-up pr.
We already using this module in production.
Introducing a version bump in this pr is undesirable, as well as adding a complex migration script for changing it withing the same version, would increase complexity for this nice to have.
IMO, better to handle it in a different pr.
|
@bosd Could you put in PR description the different improvements you've added as this is not only a migration ? Thanks |
It's possible to do these improvements here, if there is no one interested in v. 17.0 (I wouldn't personally check that version too). Anyway, other then explain it, it could lead to a slower process of the migration, so it is not always a good idea. In every case, commits of the improvements have to be squashed in 1 commit only. |
sergiocorato
left a comment
There was a problem hiding this comment.
Squash improvement commits and explain their logic in the first comment of the PR at least, no code check.
[FIX]: sale_restricted_qty: Skip quantity restriction checks for confirmed sale order lines and add a reproduction test for historical data. [IMP] sale_restricted_qty: add order name to product quantity restriction error message [IMP] sale_restricted_qty: clean up legacy code and expand tests [IMP] sale_restricted_qty: use class attribute to eliminate repetitive overrides Address PR OCA#3959 review comments: - Rename mixin to sale.product.restricted.qty.mixin - Move ensure_one() to beginning of _get_* methods - Add _sale_restricted_qty_parent_field class attribute with callable @api.depends to avoid 24+ method overrides per child model [IMP] sale_restricted_qty: use float_is_zero for modulo check and remove legacy pot - Use float_is_zero with UoM rounding for multiple-of quantity checks to avoid float precision artifacts - Remove stale sale_order_min_qty.pot from the old module name
59b427d to
5306563
Compare
|
Done, commits squashed. The work on this pr, and the one prior to it on version 17 have been open way too long. |
Maybe this is usefull for somebody. It contains the improvements from 17.0 (#2997 ) and the original migration to 18.0.
now has three layers: sale_own_* (set directly), sale_inherited_* (from parent), and sale_* (effective combined value). This replaces
the old flat manual_* fields.
product_tmpl_id) drives all 12 inherited field lookups via callable @api.depends, eliminating 24+ repetitive method overrides per
child model.
Blocking/Warning restriction mode
max, multiple-of), each with own/inherited/effective layers.
Bug fixes
Usability
Was 🤼♂️ with git, so the improvement commit author is assigned to me instead of alex.